-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added new rule require-assert-message rule #45
base: main
Are you sure you want to change the base?
Conversation
src/require-assert-message.spec.ts
Outdated
|
||
import rule from './require-assert-message'; | ||
|
||
describe('no-uuid', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update description
src/require-assert-message.spec.ts
Outdated
valid: [ | ||
{ | ||
code: `import { strict as assert } from 'node:assert'; | ||
assert.ok(statusCode === StatusCodes.OK, 'Failed to get data.');`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the indenting in the test code is weird, should be two spaces and line up correctly.
src/require-assert-message.ts
Outdated
const methodName = node.callee.property.name; | ||
|
||
if (objectName === 'assert' && methodName !== 'ifError') { | ||
const expectedMessageArgIndex = methodName === 'ok' || methodName === 'strict' ? 1 : 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am not convinced this logic is correct. e.g. don't think the assert(value)
form is accounted for? Maybe a better approach is to see if the argument named "message" is provided. (all the assert type definitions use message
as the argument name). that should be available somehow.
src/require-assert-message.spec.ts
Outdated
Error, | ||
);`, | ||
errors: [{ message: 'Missing message argument in doesNotReject() method.' }], | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a pass/fail test for every method https://nodejs.org/api/assert.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was using @typescript-eslint/utils;
somehow all the arguments are of type: 'Literal' argument and argument named ‘message’ is not available. Should we follow approach to load all assert methods in runtime from node:assert
and get all the arguments length and then report back if arguments length are less than expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be possible to do along the lines of sth like:
const parserServices = ESLintUtils.getParserServices(context);
const checker = parserServices.program.getTypeChecker();
const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node);
const signature = checker.getResolvedSignature(tsNode); /*?*/
const messageParameterIndex = signature?.getParameters().findIndex((param) => param.name === 'message'); /*?*/
but we probably need to pay attention of performance impact, it's better to somehow cache those information rather the analyzing for each assertion if we do want to go down this path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can refactor to cache instead of analyzing each assertion to improve performance. Added valid test cases for assert.ifError that doesn't have message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the current solution seems good enough, if we want to be bullet proof for any library change there is another alternative as i mentioned in the comments.
❌ PR review status - has 1 reviewer outstanding |
Beta Published - Install Command: |
Closes #30
Added rule to report for missing message argument to always be supplied to node:assert methods.